Point at argument instead of call for their obligations #88719

estebank · 2021-09-07T12:00:30Z

When an obligation is introduced by a specific fn argument, point at
the argument instead of the fn call if the obligation fails to be
fulfilled.

Move the information about pointing at the call argument expression in
an unmet obligation span from the FulfillmentError to a new
ObligationCauseCode.

When giving an error about an obligation introduced by a function call
that an argument doesn't fulfill, and that argument is a block, add a
span_label pointing at the innermost tail expression.

Current output:

error[E0425]: cannot find value `x` in this scope
 --> f10.rs:4:14
  |
4 |         Some(x * 2)
  |              ^ not found in this scope

error[E0277]: expected a `FnOnce<({integer},)>` closure, found `Option<_>`
 --> f10.rs:2:31
  |
2 |       let p = Some(45).and_then({
  |  ______________________--------_^
  | |                      |
  | |                      required by a bound introduced by this call
3 | |         |x| println!("doubling {}", x);
4 | |         Some(x * 2)
  | |         -----------
5 | |     });
  | |_____^ expected an `FnOnce<({integer},)>` closure, found `Option<_>`
  |
  = help: the trait `FnOnce<({integer},)>` is not implemented for `Option<_>`

Previous output:

error[E0425]: cannot find value `x` in this scope
 --> f10.rs:4:14
  |
4 |         Some(x * 2)
  |              ^ not found in this scope

error[E0277]: expected a `FnOnce<({integer},)>` closure, found `Option<_>`
 --> f10.rs:2:22
  |
2 |     let p = Some(45).and_then({
  |                      ^^^^^^^^ expected an `FnOnce<({integer},)>` closure, found `Option<_>`
  |
  = help: the trait `FnOnce<({integer},)>` is not implemented for `Option<_>`

Partially address #27300. Will require rebasing on top of #88546.

rust-highfive · 2021-09-07T12:00:33Z

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

src/test/ui/suggestions/slice-issue-87994.stderr

nagisa

Are there any cases where we introduce the "required by a bound introduced by this call" note on something else than the immediate method being called? So far all of the test cases seem to annotate the method and the source of the obligation seems pretty obvious. Due to this note the presentation of certain diagnostics such as the ones involving closures become fairly awkward due to the overlapping spans.

Seems good from the implementation perspective. Definitely looks cleaner than passing an argument around.

src/test/ui/associated-types/associated-types-path-2.stderr

estebank · 2021-09-13T12:41:11Z

Are there any cases where we introduce the "required by a bound introduced by this call" note on something else than the immediate method being called? So far all of the test cases seem to annotate the method and the source of the obligation seems pretty obvious. Due to this note the presentation of certain diagnostics such as the ones involving closures become fairly awkward due to the overlapping spans.

I wanted to make it clear that the reason the argument had some bound imposed on it was because of the call. This might not be needed given that have the span_note immediately afterwards in (I think) all cases, so I can remove it (or leave it with a span label with no text).

If by the overlapping case you are referring to is https://github.com/rust-lang/rust/pull/88719/files/ec904af0fc3d1dcbbd3514139e7cf6f31ce0c58b#diff-003f5fb5b17c96d98b8183c313df38486269166791f27003c3b62a4bfe251b81R8-R20, then I would say that that error is going to look ugly no matter what :-/ If we don't point to the call, that error will be less clear, IMO. If you were referring to cases like https://github.com/rust-lang/rust/pull/88719/files/ec904af0fc3d1dcbbd3514139e7cf6f31ce0c58b#diff-a6816ee609a762d3ac2a91869bb43e3d53ed29359a9259b011664dc961dcd52a, then I agree, and am somewhat surprised at the regression: the expected span should point at annotate only, not the whole expression, but thought that could be dealt with later.

estebank · 2021-09-14T11:02:23Z

src/test/ui/where-clauses/where-clauses-method-unsatisfied.stderr

+   |       ------ ^^ the trait `Eq` is not implemented for `Bar`
+   |       |
+   |       required by a bound introduced by this call


This case has a MiscObligation, so for some where clauses we need to do better, but at least we point at the relevant argument. The new span pointing at the call at least hints at the user that they should look at the equals implementation to figure out why the bound was added.

nagisa · 2021-09-15T22:07:11Z

@bors r+

bors · 2021-09-15T22:07:13Z

📌 Commit 7a8de2eca8f9b2707ec9a6ac25fcfdfc845a1227 has been approved by nagisa

bors · 2021-09-15T22:07:13Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

nagisa · 2021-09-15T22:07:26Z

@bors rollup=never

bors · 2021-09-16T08:38:22Z

⌛ Testing commit 7a8de2eca8f9b2707ec9a6ac25fcfdfc845a1227 with merge 633c54588419b027ba35ce87ff6ba15061de8ad7...

bors · 2021-09-16T08:56:24Z

💔 Test failed - checks-actions

When an obligation is introduced by a specific `fn` argument, point at the argument instead of the `fn` call if the obligation fails to be fulfilled.

Move the information about pointing at the call argument expression in an unmet obligation span from the `FulfillmentError` to a new `ObligationCauseCode`.

When giving an error about an obligation introduced by a function call that an argument doesn't fulfill, and that argument is a block, add a span_label pointing at the innermost tail expression.

When evaluating an `ExprKind::Call`, we first have to `check_expr` on it's callee. When this one is a `ExprKind::Path`, we had to evaluate the bounds introduced for its arguments, but by the time we evaluated them we no longer had access to the argument spans. Now we special case this so that we can point at the right place on unsatisfied bounds. This also allows the E0277 deduplication to kick in correctly, so we now emit fewer errors.

estebank · 2021-09-16T14:02:06Z

@bors r=nagisa

bors · 2021-09-16T14:02:08Z

📌 Commit 0a4540b has been approved by nagisa

bors · 2021-09-16T22:17:37Z

⌛ Testing commit 0a4540b with merge e366210...

bors · 2021-09-17T01:00:06Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing e366210 to master...

rust-timer · 2021-09-17T08:53:56Z

Finished benchmarking commit (e366210): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

pnkfelix · 2021-09-21T18:55:15Z

Huh. I'm doing performance triage now, and the triage report points to this PR as the source of a 1.9% regression on diesel.

There has been some associated discussion of the matter on zulip : https://zulip-archive.rust-lang.org/stream/247081-t-compiler/performance/topic/triage.202021.2021.2009.html#254207973

Anyway, it seems like this might be a legitimate regression to investigate, at least for the diesel-doc case.

@rustbot label: +perf-regression

rust-highfive assigned nagisa Sep 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2021

This comment has been minimized.

Sign in to view

estebank force-pushed the point-at-arg-for-obligation branch from 4fda83c to c343349 Compare September 7, 2021 14:05

estebank mentioned this pull request Sep 7, 2021

Emit proper errors when on missing closure braces #88546

Merged

This comment has been minimized.

Sign in to view

estebank force-pushed the point-at-arg-for-obligation branch from 52fef6d to 6219556 Compare September 12, 2021 20:37

estebank commented Sep 12, 2021

View reviewed changes

src/test/ui/suggestions/slice-issue-87994.stderr Outdated Show resolved Hide resolved

estebank force-pushed the point-at-arg-for-obligation branch from 6219556 to ec904af Compare September 13, 2021 11:03

nagisa reviewed Sep 13, 2021

View reviewed changes

src/test/ui/associated-types/associated-types-path-2.stderr Outdated Show resolved Hide resolved

estebank commented Sep 14, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 16, 2021

This comment has been minimized.

Sign in to view

estebank added 4 commits September 16, 2021 12:12

Point at argument instead of call for their obligations

284a8a9

When an obligation is introduced by a specific `fn` argument, point at the argument instead of the `fn` call if the obligation fails to be fulfilled.

Refactor FulfillmentError to track less data

8a3f712

Move the information about pointing at the call argument expression in an unmet obligation span from the `FulfillmentError` to a new `ObligationCauseCode`.

Point at call span that introduced obligation for the arg

569a842

Account for blocks in arguments

22318f1

When giving an error about an obligation introduced by a function call that an argument doesn't fulfill, and that argument is a block, add a span_label pointing at the innermost tail expression.

estebank added 6 commits September 16, 2021 12:12

fix clone call

f7c4a50

fix rebase

dbecdd5

Remove unnecessary label

88a5321

Fix rebase

1d82905

fix rebase

0a4540b

estebank force-pushed the point-at-arg-for-obligation branch from 7a8de2e to 0a4540b Compare September 16, 2021 14:01

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 17, 2021

bors merged commit e366210 into rust-lang:master Sep 17, 2021

rustbot added this to the 1.57.0 milestone Sep 17, 2021

This was referenced Sep 17, 2021

Normalize obligations for closure confirmation #88441

Merged

Suggest using Arc on !Send/!Sync types #88936

Merged

rustbot added the perf-regression Performance regression. label Sep 21, 2021

camelid mentioned this pull request Sep 28, 2021

ICE in nightly with assertion failed: !value.has_escaping_bound_vars() #89333

Closed

hkmatsumoto mentioned this pull request Oct 21, 2021

Rustc nightly points out a wrong part of line as an error #90101

Closed

estebank deleted the point-at-arg-for-obligation branch November 9, 2023 05:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Point at argument instead of call for their obligations #88719

Point at argument instead of call for their obligations #88719

estebank commented Sep 7, 2021

rust-highfive commented Sep 7, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nagisa left a comment •

edited

Loading

estebank commented Sep 13, 2021

estebank Sep 14, 2021

nagisa commented Sep 15, 2021

bors commented Sep 15, 2021

bors commented Sep 15, 2021

nagisa commented Sep 15, 2021

bors commented Sep 16, 2021

bors commented Sep 16, 2021

This comment has been minimized.

estebank commented Sep 16, 2021

bors commented Sep 16, 2021

bors commented Sep 16, 2021

bors commented Sep 17, 2021

rust-timer commented Sep 17, 2021

pnkfelix commented Sep 21, 2021

Point at argument instead of call for their obligations #88719

Point at argument instead of call for their obligations #88719

Conversation

estebank commented Sep 7, 2021

rust-highfive commented Sep 7, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nagisa left a comment • edited Loading

Choose a reason for hiding this comment

estebank commented Sep 13, 2021

estebank Sep 14, 2021

Choose a reason for hiding this comment

nagisa commented Sep 15, 2021

bors commented Sep 15, 2021

bors commented Sep 15, 2021

nagisa commented Sep 15, 2021

bors commented Sep 16, 2021

bors commented Sep 16, 2021

This comment has been minimized.

estebank commented Sep 16, 2021

bors commented Sep 16, 2021

bors commented Sep 16, 2021

bors commented Sep 17, 2021

rust-timer commented Sep 17, 2021

pnkfelix commented Sep 21, 2021

nagisa left a comment •

edited

Loading